Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Golang cgo bindings and CI tests #3

Merged
merged 1 commit into from
May 12, 2024
Merged

Golang cgo bindings and CI tests #3

merged 1 commit into from
May 12, 2024

Conversation

david415
Copy link
Member

This updates the Golang bindings originally written by David Stainton for the Katzenpost Mixnet fork and uses the latest SPHINCS+ reference implementation. This branch enhances the bindings to parameterize building, and to perform comprehensive testing for all parameter sets on GNU/Linux, MacOS, and Windows.

This updates the Golang bindings originally written by David Stainton
for the Katzenpost Mixnet fork and uses the latest SPHINCS+ reference
implementation. This branch enhances the bindings to parameterize
building, and to perform comprehensive testing for all parameter sets on
GNU/Linux, MacOS, and Windows.
@david415 david415 added the enhancement New feature or request label May 12, 2024
@david415
Copy link
Member Author

looks good to me! i appreciate that you are using the go build tags with lots of little files, it's a neat trick and the mess is siloed in the params directory. fingers cross that the maintainers of sphincsplus will be okay with all this.

@david415 david415 removed the request for review from threebithacker May 12, 2024 19:41
@david415 david415 assigned david415 and unassigned ioerror May 12, 2024
@david415 david415 requested a review from ioerror May 12, 2024 19:42
@david415 david415 merged commit 8502a19 into master May 12, 2024
324 of 360 checks passed
@david415 david415 deleted the cgo_module branch May 12, 2024 19:45
@david415 david415 restored the cgo_module branch May 12, 2024 19:48
@ioerror
Copy link
Collaborator

ioerror commented May 12, 2024

This branch was not ready to be merged as it does not yet work with CFLAGS that fit for the build tags. One consequence is that go get and go build won't work by default and every consumer of the module will have to set CFLAGS which is not really okay, even in cgo land. This is now worse for our use case than previously as katzenpost will likely not build anymore without CFLAGS in the environment. I encourage you to revert the merge so that Katzenpost won't be broken.

Things that need to be updated immediately include:

  • build tags that set the correct CFLAGS/LDFLAGS on a platform by platform basis rather than only having one set per platform and a default set of flags. This is the absolute minimum
  • a review of all build tags to ensure that they exactly match the files that are expected to build for each variant of the c library
  • a review of all symbols exported to meet exactly the expected set of symbols
  • a set of CI tests that actually exercise how we expect this to work e.g.: go install or go get
  • a small (golang) program that uses the library and prints out vectors. This program could be a go generate or template style generator but regardless, we should use it to generate our hard coded test vectors. I made the current vectors by hand and I am fairly sure I did not do this perfectly as I did not write a program to do all of the vectors systematically
  • reviewing the CFLAG differences from the c library and the generated Golang library as cgo doesn't even want you to turn on all warnings - one example is that -O3 is not enabled and there are others
  • go benchmark tests
  • increased CI coverage with several compilers
  • increased CI coverage with several additional operating systems
  • ensuring that the upstream code has no warnings or errors
  • many other small details

I will begin work on this list and I will open a pull request when I am ready to merge it. I will fold in your review comments as my first task.

@ioerror
Copy link
Collaborator

ioerror commented May 12, 2024

I have now enabled some additional tests in my branch. The CI config will now test the default ref library again, and it will perform many additional checks with gcc and clang. I will generate the build tags tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants